Repository navigation
Resolve POSIX adapter paths against the base path - #13
Closed
jamiecounsell wants to merge 2 commits into
Closed
jamiecounsell wants to merge 2 commits into
jamiecounsell wants to merge 2 commits into
Conversation
rmdir() in the POSIX and stdio adapters returned false when the directory was removed and true when it wasn't, the opposite of every other adapter (and of mkdir()). Adds test/test_posix_fs, which runs the adapters against a real temp directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rUgoKJdB4U8fNS9HybY8S
The POSIX adapter took a base path but ignored it (outside ESP32's LittleFS.begin()), so relative store paths like ./known_store/ resolved against the working directory. On ESP-IDF there is no working directory, so these paths never resolved. The adapter also kept the caller's pointer, which dangles if the string was only temporary. The adapter now keeps its own copy of the base path and resolves relative paths against it. Absolute paths, and everything when the base path is empty (the default), are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rUgoKJdB4U8fNS9HybY8S
Owner
|
Thanks for this, and for the tests and the detailed write-up. The problems you found are real: the POSIX adapter ignores its base path, relative store paths have nothing to resolve against on ESP-IDF, and the adapter holds on to the caller's pointer. They need fixing. I'm going to close this PR, though, because I don't think the fix should be made one adapter at a time. There are two reasons:
The right fix is a single path model shared by every adapter, so a store path means the same thing whichever adapter is underneath. I'll fix it that way. Your ESP-IDF case (no working directory, relative store paths) is a good test for it. Thanks again for digging into this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: this PR is stacked on #12
The POSIX adapter took a base path parameter but ignored it (outside ESP32's
LittleFS.begin()), so relative store paths like./known_store/resolved against the working directory. On ESP-IDF there is no working directory, so these paths never resolved. The adapter also kept the caller's pointer, which dangles if the string was only temporary.Now, the adapter keeps its own copy of the base path and resolves future relative paths against it. Absolute paths, and everything when the base path is empty (the default), remain unchanged.
The stdio adapter has the same unused base path. I left it alone to keep this change to one adapter.
Testing:
pio test -e native: the new base-path tests pass and fail without the fix. The same threetest_file_storetests fail as on master.heltec_wifi_lora_32_V3builds the adapter with-DUSTORE_USE_POSIXFS. Running in SolarOS on the M9 as the store backend for microReticulum.